Skip to content

refactor(apple): carry one phase Deadline through the runner interfaces and test cancellation as a matrix - #2473

Merged
thymikee merged 1 commit into
mainfrom
claude/runner-phase-deadline-matrix
Sep 10, 2026
Merged

refactor(apple): carry one phase Deadline through the runner interfaces and test cancellation as a matrix#2473
thymikee merged 1 commit into
mainfrom
claude/runner-phase-deadline-matrix

Conversation

@thymikee

Copy link
Copy Markdown
Member

Follow-up to #2423, on the two points raised in review: encode one shared Deadline per runner phase in the owning interfaces, and turn #2423's ad-hoc cancellation cases into a matrix.

Behavior is unchanged. This is an interface + test change.

1. One phase budget at the boundary

#2423 fixed the symptom (a cold toolchain-probe stall no longer adds its 30-45 s on top of the phase budget) but left the shape that produced it: each phase entry point took a number, converted it to a Deadline internally, and then handed the deadline plus a signal onward in a separate bag - while the original number stayed in scope and reachable. Adoption took both startupTimeoutMs and phaseDeadline; requireRunnerPhaseRemainingMs took the deadline and a numeric fallback. A number that two callees can each turn into a budget is exactly the double-spend class.

Now a phase is opened once, from the public numeric option, and every boundary below it takes that one object.

Before:

export function createRunnerPhaseDeadline(timeoutMs: number | undefined): Deadline | undefined;
export function requireRunnerPhaseRemainingMs(
  deadline: Deadline | undefined,
  fallbackTimeoutMs: number | undefined,
  phase: string,
): number | undefined;

type RunnerCacheProbeBudget = { deadline?: Deadline; signal?: AbortSignal };

ensureXctestrunArtifact(device, { buildTimeoutMs?: number; signal?: AbortSignal; ... });
tryAdoptRunnerSessionFromLease(device, {
  startupTimeoutMs?: number;
  phaseDeadline?: Deadline;
  signal?: AbortSignal;
  expectedRunnerSessionId?: string;
});
resolveExpectedRunnerCacheMetadata(device, projectRoot?, budget?: RunnerCacheProbeBudget);

After:

/** The single clock every step of one phase reads, plus the owning request's cancellation. */
export type RunnerPhaseBudget = Readonly<{ deadline?: Deadline; signal?: AbortSignal }>;

/** The one place a number becomes a budget. */
export function createRunnerPhaseBudget(
  timeoutMs: number | undefined,
  signal: AbortSignal | undefined,
): RunnerPhaseBudget;

/** Still the one "what is left, or fail before spawning" reader - no numeric fallback to re-read. */
export function requireRunnerPhaseRemainingMs(
  budget: RunnerPhaseBudget | undefined,
  phase: string,
): number | undefined;

ensureXctestrunArtifact(device, { budget?: RunnerPhaseBudget; ... });
tryAdoptRunnerSessionFromLease(device, {
  budget?: RunnerPhaseBudget;
  expectedRunnerSessionId?: string;
});
resolveExpectedRunnerCacheMetadata(device, projectRoot?, budget?: RunnerPhaseBudget);
  • Public numeric options stay where external callers set them: ensureRunnerSession still takes the request-level startupTimeoutMs and buildTimeoutMs, and is the one owner that converts - startup once, and the build as its own phase (the build must not be capped by what startup has left, as before). The cache prewarm in runner-client.ts opens the build phase it starts.
  • createRunnerPhaseDeadline is gone: its two call sites became one factory that also carries the request signal, so the clock and the cancellation a phase is read against can no longer be paired up wrongly.
  • The signal rides in the budget because both are read at the same guard (ToolchainProbeClock.throwIfCanceled / attemptTimeoutMs), and startRunnerSessionWithLease no longer re-resolves it.
  • Dropping the numeric fallback is behavior-preserving: a caller with no timeout number has no deadline either, and normalizeRunnerStartupTimeoutMs already discarded non-finite values.

2. Cancellation as a matrix

runner-cache-metadata.test.ts - one table-driven test over resolveExpectedRunnerCacheMetadata, each row naming when the request aborts, what the probe it lands on does, the expected outcome, the exact exec count and the wall clock spent:

row expected execs
aborted before the first probe, cold cache request canceled 0
aborted before the first probe, fingerprint cache warm request canceled 0
aborted while the first probe blocks, and it then times out request canceled 1
aborted while the first probe fails with a non-timeout error request canceled 1
aborted as the first attempt's timeout unwinds, before its retry request canceled 1
aborted while the final probe fails with a non-timeout error request canceled 3
aborted while the final probe blocks, and it then times out request canceled 3
never aborted, the first probe and its retry spend the whole budget budget exhausted, not an unreadable toolchain 2
never aborted, the first probe times out and its retry recovers success 4

snapshot-source/cache-identity.test.ts - the same table over its smaller surface: aborted before the deadline is opened (0 execs), aborted while the first probe blocks and times out, aborted while it fails with a non-timeout error (the probe's own failure: a non-timeout failure is never retried, so there is no retry to cancel), aborted as the timeout unwinds before the retry, and no abort with the whole deadline spent (the timeout propagates unchanged).

The named tests that cover non-cancellation behavior are kept as they were: budget sharing across the retry ([30 s, 15 s]), a phase with 4 s left getting one 4 s attempt, and a probe whose own message merely says "timed out" not being retried.

Removing the cancellation guard from createToolchainProbeClock fails exactly the 7 abort rows and leaves the 2 non-abort rows green.

Verification

typecheck (packages clean), lint, check:layering, check:affected --run, scripts/__tests__/eager-closure-budgets.test.ts (no growth), and the four touched test files.

Note: main at 6d08de4 does not typecheck - --until (#2436) landed without the projectConfig/recorded fields that #2453 made required on FlagDefinition, in src/commands/cli-grammar/flag-definitions-action.ts. That failure is inherited, not from this branch, and clears on a rebase once main is fixed.

@github-actions

github-actions Bot commented Sep 10, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.54 MB 4.54 MB -194 B
Package (unpacked) 4.54 MB 4.54 MB -194 B
Package (download) 1.35 MB 1.35 MB -50 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.0 ms 25.9 ms -1.1 ms
CLI --help 76.9 ms 72.5 ms -4.3 ms

@thymikee

Copy link
Copy Markdown
Member Author

No new code findings at fb52986. The refactor preserves the existing phase boundaries while passing deadline and cancellation together; the cancellation matrix checks outcomes, elapsed budget and probe counts. The inherited until flag typing error is fixed on main by #2472, but this head still needs CI with that fix included. Marking ready for human review. One pre-existing limitation remains outside this refactor: adoption's uptime probe uses a fixed 500 ms timeout without the request signal.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 10, 2026
@thymikee
thymikee force-pushed the claude/runner-phase-deadline-matrix branch from fb52986 to 37646f7 Compare September 10, 2026 16:15
@thymikee
thymikee merged commit f4c8f3d into main Sep 10, 2026
16 of 18 checks passed
@thymikee
thymikee deleted the claude/runner-phase-deadline-matrix branch September 10, 2026 16:44
@thymikee

Copy link
Copy Markdown
Member Author

The reviewed patch is unchanged at 37646f7 and remains code-clean. CI now identifies one related fix: runner-client.test.ts grew from 1,441 to 1,442 lines and fails the test-size ratchet. Move the touched artifact tests to the module they exercise rather than weakening the gate. The separate iOS smoke failure is a wait timeout; its cause is unclear. Keeping the human-review label, but CI is not green.

@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-10 16:46 UTC

@thymikee

Copy link
Copy Markdown
Member Author

Suspected of breaking the iOS live lanes on main (Settings replay smoke and fixture E2E smoke: waits time out after the AX-bridge prepare is cancelled and the runner fallback retries without answering). Every CI run containing this change failed those lanes; runs without it in the same window passed. Reverting first (see the revert PR), then fixing forward with a live reproduction.

@thymikee

Copy link
Copy Markdown
Member Author

Retracting the suspicion above: the local reproduction did not flip between this commit and its parent (4/4 green on both, identical request logs), the budgets in the red CI logs are byte-identical to green runs, and two of the four correlated failures were a Swift-only step on a different Xcode image. Revert #2474 is closed unmerged. The underlying flake (a 1 s readiness preflight abandoning a runner command that keeps the runner busy ~5 s) is filed separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant